feat(gno.land): support setting custom height/timestamp for genesis txs #2751
Conversation
github-actions
bot
added
📦 🤖 gnovm
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2751 +/- ##
==========================================
+ Coverage 60.83% 60.86% +0.03%
==========================================
Files 563 563
Lines 75169 75121 -48
==========================================
- Hits 45730 45724 -6
+ Misses 26072 26034 -38
+ Partials 3367 3363 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I think we have to be careful with that, because we are representing block heights as unsigned ints everywhere. |
|
For the context, here is the previous discussion with the relevant background information: #1511 |
Ideally, I suggest we sunset
This solution could work, but it sounds rather hacky and may not allow for indexers to properly index the data. Instead, let's try to preserve the history if possible, and use this alternative approach only as a fallback. Note that @gfanton mentioned that replying with block history works, but it is slow. Perhaps we could introduce a custom flag to enable a "fast and inconsistent" mode for local development, or a "slow but more consistent" mode for environments that require preserving the correct height. Another consideration is for users who do rely on the height with strict accuracy, such as those using it for random seed initialization or certain DAO patterns. For example, @ilgooz depends on the height for voting, and being negative may not be a problem if we maintain the same "gap." However, it would be preferable to try to preserve the correct height if possible.
I need to devote more time to considering this matter. We can discuss it during our next retreat if it hasn't been addressed already. |
okay, I'll look to see if there is a good way we can replay blocks in order, and keep heights consistent. |
|
Ideally, indexers like gnoscan could expect a reliable block height while having a special flag like "--ignore-invalid-blocks" to avoid strictly requiring all blocks and correct hashes when processing a reply. The devnet is a special environment, not a real blockchain, with a single node, so peer-to-peer synchronization is not a priority. For contract writers and tools, a consistent execution context, such as block height and timestamp, would be more crucial. |
|
I was considering adding a feature that could facilitate analysis from tools, if it's cost-effective to implement. Otherwise, let's keep it minimal. If feasible, we could include metadata in the exports specifying whether the transaction was successful or not. |
| // BlockHeight as the one used by the previous iteration of the chain, as | ||
| // from the realm's point of view it will eventually be as if a height | ||
| // occurred twice at different times: instead, use negative heights. | ||
| // (Last block should be -1, as 0 is the genesis block). |
There was a problem hiding this comment.
This isn't very clear to me. Is this saying that transactions will be executed by order of height? So, for example, -5, -4, -3, -2, -1, 0 -- genesis for unspecified transactions?
There was a problem hiding this comment.
Not necessarily. What I'm trying to communicate is that the keeper/gno.land does not actually "control" the Height; but to avoid collisions with the blocks that are created after genesis, the "space" that should be used for heights should be for negative ones.
But really, this is something that the users of this feature would need to set up with the corresponding values in genesis.json. ie. to guarantee ascending block heights, just order the transactions in a way so that they're ascending.
| // occurred twice at different times: instead, use negative heights. | ||
| // (Last block should be -1, as 0 is the genesis block). | ||
| Height int64 `json:"height"` | ||
| Timestamp int64 `json:"timestamp"` |
There was a problem hiding this comment.
Is it required to have both height and timestamp? Could we just have height and derive timestamps for each height? There could be some wonky behavior here if the tx at height -4 has a timestamp after the tx at height -1, or swap height for timestamp if we choose it replay transactions by timestamp rather than height.
There was a problem hiding this comment.
Ideally not.
Timestamp != genesisTime + blockHeight * blockTime, because there may be multiple consensus rounds; so it has to be incremented by failedRounds * blockTime at least.
So I'd keep them separate.
There was a problem hiding this comment.
I understand that is not how timestamp is computed, but what do failed rounds have to do with executing transactions with negative heights? Won't they be executed before genesis? Are there consensus rounds for this? When I asked about deriving timestamps, I was referring to deriving timestamps for transactions with negative heights. I'm just trying to understand the negative heights and how or if there is any relationship between negative height and timestamp and if a transaction with an earlier timestamp can be executed before a transaction with a later timestamp.
There was a problem hiding this comment.
When I asked about deriving timestamps, I was referring to deriving timestamps for transactions with negative heights. I'm just trying to understand the negative heights and how or if there is any relationship between negative height and timestamp and if a transaction with an earlier timestamp can be executed before a transaction with a later timestamp.
So, we likely won't be going down the road of negative block heights (the current state of this PR), but I'll still try to answer your question.
This PR per se does not define when a transaction with negative height should happen. It could be at any point in time; the only consequence is, of course, that some realms will expect ascending order for both timestamps and block heights, and have code work under this assumption.
For the use case I envisioned of this feature, though, I wanted to have the result of time.Now() essentially be always the same on the portal loop. So that your transaction is first run on the portal loop at height=10 time=2024-09-12T18:30:00Z; then when the portal loop restarts and all the transactions are dumped into the genesis file, the transaction is run again in the InitChainer with height=-X time=2024-09-12T18:30:00Z; although it's not the "real" block time and height, but rather an emulated one, injected through the TxContexts field in genesis.json
Just to clarify, we are using timestamps, in case it makes sense to split the issue in two, initially supporting timestamps and eventually implement support for heights. |
## Description This PR introduces metadata support for genesis transactions (such as timestamps), in the form of a new Gno genesis state that's easily generate-able. Shoutout to @clockworkgr for sanity checking the idea, and providing insights that ultimately led to this PR materializing. **BREAKING CHANGE** The `GnoGenesisState` is now modified, and all functionality that references it (ex. `gnogenesis`, `tx-archive`) will need to adapt. ### What we wanted to accomplish The Portal Loop does not save "time" information upon restarting (from block 0). This means that any transaction that resulted in a Realm / Package calling `time.Now()` will get differing results when these same transactions are "replayed" as part of the loop (in the genesis state). We wanted to somehow preserve this timestamp information when the transactions (from a previous loop), are executed as part of the genesis building process. For example: - Portal Loop chain is on block 100 - tx A results in a call to `time.Now()`, which returns time T, and saves it somewhere (Realm state) - the Portal Loop restarts, and uses all the transactions it encountered in the past iteration as genesis txs - the genesis is generated by executing the transactions - when tx A is re-executed (this time as part of the genesis block), **it should return time T, as with the original execution context** It is worth noting that this functionality is something we want in `gnodev`, so simple helpers on the Realms to update the timestamps wouldn't work. ### What this PR does I've tried to follow a couple of principles when working on this PR: - don't break backwards compatibility - don't modify critical APIs such as the SDK ABCI, but preserve existing, working, functionality in its original form - don't add another layer to the complexity pancake - don't implement a solution that looks (and works) like a hack I'm not a huge fan of execution hooks, so the solution proposed in this PR doesn't rely on any hook mechanism. Not going with the hook approach also significantly decreases the complexity and preserves readability. The base of this solution is enabling execution context modification, with minimal / no API changes. Having functionality like this, we can tailor the context during critical segments such as genesis generation, and we're not just limited to timestamps (which is the primary use-case). We also introduce a new type of `AppState`, called `MetadataGenesisState`, where metadata is associated with the transactions. We hide the actual `AppState` implementation behind an interface, so existing tools and flows don't break, and work as normal. ### What this PR doesn't do There is more work to be done if this PR is merged: - we need to add support to `tx-archive` for supporting exporting txs with metadata. Should be straightforward to do - the portal loop also needs to be restarted with this new "mode" enabled - we need to add support to existing `gnoland genesis` commands to support the new `MetadataGenesisState`. It is also straightforward, but definitely a bit of work - if we want support for something like this in gnodev, the export / import code of gnodev also needs to be modified to support the new genesis state type (cc @gfanton) - #2943 Related PRs and issues: - #2751 - #2744 cc @moul @thehowl @jeronimoalbi @ilgooz <details><summary>Contributors' checklist...</summary> - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md). </details> --------- Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
this is a small PR to allow support for setting
TxContextsin the genesis transaction files. Request from #2744:This PR does preparatory work to set that up. With this set up, which has no effect on those who don't use the specific feature (setting
tx_contexts), genesis transactions can be executed using custom timestamps and block heights.To achieve the objective, additional work needs to be carried out on tx-archive, tx-exports and misc/loop, to:
TxContextsin thegenesis.jsonfile created at each iteration of the loop.Note: with this approach, portal loop restarts will still start with genesis height = 0, then start the chain with height = 1. The idea here is that BlockHeights for genesis transactions can also be set to negative values. This means that relative block heights are valid, while their absolute value might change. (Ie.
blockHeight1 - blockHeight2, from two different, possibly distant, invocations ofstd.GetHeight(), will yield the same result; but the specific value ofstd.GetHeight()changes on portal loop restarts).Implementation
This code uses
issue-2283(#2319) as a base, to take advantage of theBeginTxHookintroduced in that PR. TheInitChainerConfighas a field,beginTxHook, that is set each time a genesis transaction is processed. This allows us to inject into the context theExecContextCustom. Following that, the unified VM KeepernewExecContextwill check thesdk.Contextto see if there's aExecContextCustom(only at startup).Am I particularly proud of this? No, if you can think of a better approach which can have a very simple implementation (note: we need this feature ideally within the next week) and allows us to change the height and timestamp I'm all ears.
One I can think of is to change the context's BlockHeader, putting the values we want for Height/Timestamp; thus changing them at the "tm2" level, rather than at the GnoVM level. This should be harmless; but there's no way to predict and detect misuse (ie. using
ctx.BlockHeader().Height(), asserting that this value is provided by the chain and never by the genesis/end-user somehow, which would be the case here). So I'm more tempted to do this.I attempted to place the values
timestampandheightby trying to create a struct that embedsstd.Txand extends it with the two new fields (so as not to touch the normal std.Tx) but unfortunately am̸̭̈ino JSON does not support embedding a struct into another likeencoding/jsondoes, so they're just included in the resulting JSON as if they were a normal field. So yes, that road's Too Complex; a different array will have to do the trick 🥲